Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

loosen number index check, fixes #15768 #17767

Merged

Conversation

KiaraGrouwstra
Copy link
Contributor

@KiaraGrouwstra KiaraGrouwstra commented Aug 13, 2017

see title


Edit by @DanielRosenwasser: Fixes #15768

@msftclas
Copy link

@tycho01,
Thanks for having already signed the Contribution License Agreement. Your agreement has not been validated yet. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

const typeOrConstraint = (tp: Type) => maybeTypeOfKind(tp, TypeFlags.TypeVariable) ? getBaseConstraintOfType(tp) || tp : tp;
if (isTypeAssignableToKind(typeOrConstraint(indexType), TypeFlags.NumberLike) &&
getIndexInfoOfType(typeOrConstraint(objectType), IndexKind.Number)) {
return type;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this to just:

if (getIndexInfoOfType(getApparentType(objectType), IndexKind.Number) && isTypeAssignableToKind(indexType, TypeFlags.NumberLike)) {
    return type;
}

Otherwise PR looks good. Thanks!

@KiaraGrouwstra
Copy link
Contributor Author

Fixed. That's definitely even cleaner! 😅

@ahejlsberg ahejlsberg merged commit a1cbeb2 into microsoft:master Aug 14, 2017
@gcnew
Copy link
Contributor

gcnew commented Aug 15, 2017

I might be late to the party but this doesn't seem right. When the index is out of range, the result is a union of all constituents. While that's how expression level tuples work, it's not correct and we shouldn't spread misleading behaviour to the type level.

type X<I extends number> = ['a'][I];
type Test = X<1>// 'a' :/

@KiaraGrouwstra
Copy link
Contributor Author

@gcnew: see also type X = ['a'][1]; // 'a'. I guess it'd be hard to remove its number index while retaining it for the expression level though, presuming you wouldn't just remove it for both altogether.
A related concern I have is tuple type -> array type assignability mentioned at #17785, which could unblock inferring tuples rather than always widening them.

On another note, I do still get a few Type <generic> cannot be used to index type <array> errors; I'll see if I can make a minimum repro.

@KiaraGrouwstra KiaraGrouwstra deleted the 15768-generic-numeric-index-error branch August 15, 2017 04:39
@gcnew
Copy link
Contributor

gcnew commented Aug 15, 2017

@tycho01 Yes, you are right. Unfortunately it's been broken and you've made it more consistent. In this case 👍.

@KiaraGrouwstra
Copy link
Contributor Author

KiaraGrouwstra commented Aug 20, 2017

@gcnew: I gave the issue you raised a bit more thought.

When the index is out of range, the result is a union of all constituents. While that's how expression level tuples work, it's not correct and we shouldn't spread misleading behaviour to the type level.

I think the root question here is what the index types are for. There appear to be three use-cases:

  • ['a'][1]: expected result might be either an error or equivalent to the expression variant, undefined
  • ['a'][number], intending to get the fallback value, see above. terrible example as ['a'][-1] would cut it consistently, but then again they resolve by the same mechanism.
  • ['a'][number], intending to get a union of all good values. workaround exists.
  • ['a'][number], intending to get a union of all possible return values, both good and fallback (see above).

I do suppose all of this only goes to show you're right that current behavior for tuples may not be ideal.
For object types it's actually perfect; it errors so property access is safe, while getting all good values or a fallback is trivial as well.
For tuple types on the other hand, due to the current behavior getting a fallback value now requires... explicit checks.

I'm not surprised about the current behavior in the sense it just got this index behavior from Array, though it could technically overwrite to return a more narrow type, as I suggested for length.

Breaking changes would probably all get flags if they get in at all, and I do wonder about the potential impact (both positive and negative) of such a change -- or what the desired behavior should be at all.
Power users will find ways how to do anything anyway, while it seems tough to optimize for casual users at all, except maybe by never changing behavior -- it's anyone's guess which of the above potential use-cases of indices they might expect to be able to use them for anyway.
It isn't obvious what they 'should' be for to me either, except homogeneous Object and Array needed them and tuples just kinda happened to be arrays in JS.

Not that I oppose changes, hopefully (under flags) we can add in all the more ideal behavior.

tl;dr elaborate? what would you prefer and why?

Edit: to explain my confusion, the alternatives each have issues:

  • undefined reflects JS behavior for ['a'][1], yet clashes the sub-type requirement of Array<union>
  • never sub-types it but doesn't reflect behavior of ['a'][1] and is dangerous (in RHS it's an any)
  • erroring on e.g. ['a'][1] though not on ['a'][number] (leaving the latter with its current behavior, or even never) seems... viable, I guess?

Do those last ideas match what you'd been hoping for there? I'm pretty neutral here, just curious.

@gcnew
Copy link
Contributor

gcnew commented Aug 20, 2017

I think it would have been best if tuples behaved like object with their indices as keys. All the normal object rules should be in effect. For example, the indexing type variable should be a keyof of the tuple, or the tuple should be intersected with an indexer to provide a default value.

// ['syntax', true] => { 0: 'syntax', 1: true }
type Tup = ['syntax', true]
type T1<I> = Tup[I] // error
type T2<I extends number> = Tup[I] // error
type T2<I extends keyof Tup> = Tup[I] // OK
type T2<I extends number> = (Tup & { [k: number]: undefined })[I] // OK

On a side note, I don't think tuples should be a subclass of Array. In the general sense, they are not arrays, just values packed together. That's further exacerbated by the fact that tuples are heterogenous in the majority of cases.

@KiaraGrouwstra
Copy link
Contributor Author

I don't think tuples should be a subclass of Array. In the general sense, they are not arrays, just values packed together.

I guess, yeah.

Complication from recent experience: in my fixed length PR I originally had tuples extend a new Tuple interface extending ReadonlyArray instead of Array.
That just made things not compile anymore, yielding some assignability errors regarding lack of push method and the like until I had it extend Array again instead.

Unfortunately I squashes out those commits though, and I guess a couple methods already aren't compatible; there may be a way I guess.
Tuple should benefit from the decoupling too -- the indexOf variant it's inheriting off Array is utterly useless on [1,2,3] (can't search for a number as that isn't within T for Array<1|2|3>).

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
@KiaraGrouwstra KiaraGrouwstra restored the 15768-generic-numeric-index-error branch September 14, 2018 18:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants